Skip to content

Share one Kit app across test files instead of booting it per file - #6853

Draft
mataylor-nvidia wants to merge 10 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/kit-test-markers
Draft

Share one Kit app across test files instead of booting it per file#6853
mataylor-nvidia wants to merge 10 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/kit-test-markers

Conversation

@mataylor-nvidia

@mataylor-nvidia mataylor-nvidia commented Aug 2, 2026

Copy link
Copy Markdown

Problem

Kit-dependence is currently a property of importing a test file. 156 test modules construct AppLauncher at module scope, so Isaac Sim boots during pytest collection:

simulation_app = AppLauncher(headless=True).app

Nothing declares that dependency, so tools/conftest.py compensates by running every test file in its own subprocess. Kit startup is therefore paid once per file, and there is no way to ask "which tests actually need Kit?" without importing them.

Measured cost

A temporary CI probe ran the same 23 files two ways on the same commit and image:

per-file batched delta
Job total 9m47s 5m29s −44%
Test step 9m32s 5m13s −45%
Kit boots 23 1

Per-file breakdown from the baseline, showing the overhead is startup rather than tests:

File Test time Wall time
test_mass_fragments.py 0.12s 17.77s
test_utils_stage.py 0.22s 17.78s
test_simulation_context.py 63.63s 81.33s

Kit boot plus process spawn costs a consistent 17.6–18.7s per file, which is 79–85% of wall time across the directory. The probe jobs have been removed now that the number is recorded.

Change

launch_kit() (isaaclab/test/launch.py) replaces the module-scope AppLauncher. It is idempotent: the first module in a process boots Kit, later modules get the running app. It stays at module scope because a test module's own imports (pxr, omni, ...) run at collection, before any fixture could help.

kit / kit_cameras / kitless / kit_solo markers let a file declare its launch configuration, so files that can share a process are grouped without importing them.

tools/_kit_batching.py groups same-profile files into one pytest invocation and demultiplexes the resulting JUnit report back per file, so the summary table, failed-file list, and uploaded artifact stay keyed by file. Enabled by ISAACLAB_TEST_BATCH_KIT=1; the per-file path is unchanged and remains the default.

test_kit_marker_contract.py keeps the markers from drifting by checking, via AST, that each file's declaration matches what it does at module scope. AST rather than text because several kit-free files mention AppLauncher only in a docstring saying they do not use it — a grep flags those, an AST walk does not.

conftest.py is unchanged.

The two profiles cannot be merged

kit and kit_cameras are mutually exclusive, not nested. Cameras cannot be enabled after startup, which rules out one order; and test_simulation_context.py::test_headless_mode asserts not sim.has_offscreen_render, which rules out the other. launch_kit() raises on any mismatch rather than returning an app configured differently from what the marker declares.

Files that cannot share a process

Found by the probe, marked kit_solo:

  • test_views_xform_prim.pytest_compare_get_world_poses_with_isaacsim goes through Isaac Sim's SimulationManager, a process-global singleton caching the PhysxScene for /physicsScene. In a shared process that prim belongs to a stage an earlier file tore down, so the test dies with Accessed invalid expired 'PhysicsScene' prim.
  • test_simulation_stage_in_memory.py — aborts the interpreter immediately after collection with no traceback. Cause not yet understood.

Also excluded from batching: unmarked files, device_split files, node-ID-selected files, the visualizer files retried in a fresh process, and anything whose own timeout reaches 2000s. Batching is disabled under the work queue. When a batch dies early, the files it never reached are re-run individually, so batching degrades to the behaviour it replaces.

Scope

source/isaaclab/test/sim is migrated as the pilot: 23 kit, 3 kit_cameras, 2 kit_solo. The other ~125 files across other packages are untouched — an unmarked file is treated as legacy and still gets its own process. _ENFORCED_ROOTS in the guard is empty, so no file is yet required to carry a marker; it grows per package as migration proceeds.

tools/codemods/kit_launch_migration.py applies the transform. It edits line ranges in place rather than round-tripping through ast.unparse, and refuses anything it cannot rewrite without changing behaviour — including conditional launches like AppLauncher(...).app if _USE_KIT else None, which two converter tests use so they can run kitlessly.

Incidental fixes

  • test_operational_space.py assigned pytestmark twice, so the second assignment discarded arm_ci and the file had been excluded from the ARM CI lane. Found by the guard on its first run.
  • The cold-shader-cache buffer stopped applying to migrated camera files. tools/conftest.py granted the first camera test an extra 700s by grepping for the literal enable_cameras=True, which migration removes. The file was then killed at the 120s startup deadline. Now matches the marker and the launch_kit call as well.

Testing

  • test_kit_marker_contract.py (8 tests) and test_kit_batching.py (33 tests) pass in ~2s with no Kit. Both had their teeth verified by deliberately breaking the logic they guard.
  • Grouping verified against the real directory: 43 files → 19 Kit boots.
  • uv run isaaclab -f clean.
  • The migrated files themselves cannot run on the authoring machine (no Isaac Sim); CI is their first execution, which is what the probe was for.

🤖 Generated with Claude Code

@github-actions github-actions Bot added isaac-lab Related to Isaac Lab team infrastructure labels Aug 2, 2026
Kit-dependence is currently a property of importing a test file: 156 test
modules construct AppLauncher at module scope, so Isaac Sim boots during
pytest collection. Because nothing declares that dependency, tools/conftest.py
has to run every test file in its own subprocess, paying Kit startup once per
file.

Introduce the two pieces needed to change that:

launch_kit() is an idempotent module-scope replacement for AppLauncher. The
first test module in a process boots Kit; later modules receive the running
app, so a pytest run covering several files pays startup once. It raises
rather than silently returning a mismatched app when a file asks for cameras
after a camera-less boot.

The kit / kit_cameras / kitless markers let a file declare which launch
configuration it needs, so files that can share a process can be grouped
without importing them. kit_solo opts a file out of any such grouping.

test_kit_marker_contract.py keeps the markers from drifting: it checks by AST
that a file's declaration matches what it does at module scope. The checks are
AST-based rather than text-based because several kit-free files mention
AppLauncher only in a docstring saying they do not use it. Files are not yet
required to carry a marker; _ENFORCED_ROOTS is empty and grows per package as
files are migrated.

No test file changes behaviour: nothing is marked kit or kitless yet, and no
file calls launch_kit() yet.

The guard found one pre-existing bug on its first run. test_operational_space
assigned pytestmark twice, and the second assignment discarded arm_ci, so the
file had been excluded from the ARM CI lane. Merged into a single list.
@mataylor-nvidia
mataylor-nvidia force-pushed the mataylor/kit-test-markers branch from 8c337c5 to 57ffdbd Compare August 2, 2026 20:33
Replace the module-scope AppLauncher construction in the Kit-dependent files
under source/isaaclab/test/sim with launch_kit(), and declare the matching
kit or kit_cameras marker on each file.

Because launch_kit() is idempotent, a pytest process covering several of
these files now boots Kit once instead of once per file. Nothing forces them
into one process yet -- tools/conftest.py still runs a subprocess per file --
so this changes how the files launch Kit, not how CI schedules them.

24 files map to `kit` and 4 to `kit_cameras`. The two groups must not share a
process in that order: a camera-enabled app can serve tests that do not need
cameras, but cameras cannot be enabled after startup, so launch_kit() raises
rather than handing back an app that would silently fail to render.

The transform is applied by tools/codemods/kit_launch_migration.py, added
here because ~125 files in other packages remain to migrate. It edits line
ranges in place rather than round-tripping through ast.unparse, which would
discard comments and isort directives, and it preserves each launch call's
position so the Kit-dependent imports below it still run after Kit starts.

The codemod refuses anything it cannot rewrite without changing behaviour,
and reports it. In particular it rejects a conditional launch such as
`AppLauncher(...).app if _USE_KIT else None`, which test_mjcf_converter.py
and test_urdf_converter.py use so they can run kitlessly when the standalone
importer wheel is installed; collapsing that ternary would have made the boot
unconditional. It also refuses a file that references AppLauncher for
anything other than the launch call, since the import is removed.
Whether migrating the remaining ~125 test files off module-scope AppLauncher
is worth doing depends on how much Kit startup actually costs, which is not
something the current pipeline reports directly.

Add two temporary jobs that run the same 30 files from
source/isaaclab/test/sim and differ only in how many Kit apps they boot.
kit-reuse-probe-per-file keeps the default test-path of "tools", so
tools/conftest.py gives each file its own subprocess and Kit boots 30 times.
kit-reuse-probe-batched points pytest at the files directly, so they share
one process and launch_kit() boots Kit once. The difference between the two
job durations is what reuse is worth per 30 files.

Both jobs list their files explicitly instead of selecting with `-m kit`,
because pytest's marker filtering deselects tests but still imports every
collected module, and importing a kit_cameras module calls
launch_kit(cameras=True) regardless of whether its tests will run. The
batched job lists the four kit_cameras files first: a camera-enabled app can
serve tests that do not need cameras, but cameras cannot be enabled after
startup, so the opposite order makes launch_kit() raise. Files in
TESTS_TO_SKIP are excluded from both sides so the jobs cover the same tests.

To let a job bypass the per-file orchestrator, run-package-tests gains a
test-path input. It defaults to "tools", the value that was previously
hard-coded, so every existing caller is unaffected.

Both jobs are continue-on-error and are meant to be deleted once the
measurement is recorded.
@mataylor-nvidia
mataylor-nvidia force-pushed the mataylor/kit-test-markers branch from 57ffdbd to ded9dfd Compare August 2, 2026 22:19
The per-file runner grants the first camera-enabled test file an extra 700 s
of timeout, because that file compiles RTX shaders (~600 s) on a cold cache.
It identified such files by searching their source for the literal string
"enable_cameras=True".

Migrating a file to launch_kit(cameras=True) removes that literal, so the
buffer stopped being applied and the file was killed at the 120 s startup
deadline instead. That is what happened to
test_simulation_stage_in_memory.py in the kit-reuse-probe-per-file job: it
was reported as a startup hang at 120.94 s having run no tests.

Match the marker and the launch_kit call as well as the old literal, so the
buffer applies both before and after a file is migrated.

Also narrow the probe to the 24 `kit` files and drop the four `kit_cameras`
ones from both sides. The cold shader compile is roughly thirty times the Kit
startup the probe is trying to measure, so including those files tells us
about shader caching rather than about app reuse.
The kit-reuse-probe-batched job surfaced two ways a test file can misbehave
once it no longer has a Kit process to itself.

test_simulation_stage_in_memory.py aborted the interpreter immediately after
collection, with no Python traceback, while the same test passes in its own
process. Creating the stage in memory is sensitive to what else has already
touched the stage or the extension set. The cause is not understood yet, so
mark the file kit_solo to keep it out of any future batching rather than
leave a landmine for whoever wires that up.

test_views_xform_prim.py calls enable_extension() at module scope, so in a
shared process it mutates the running app's extension set during collection,
before any test runs. That is harmless today and the file is not being
changed, but it is the kind of import-time side effect that batching turns
into a cross-file interaction, so say so at the call site.

Neither change affects how these tests run today; both files still get their
own process from tools/conftest.py.
The batched probe was passing over files rather than classifying them: the
four kit_cameras files had been dropped from both sides to keep the
measurement clean, which meant three files that can share an app were being
run one-app-each for no reason.

Mark the files that genuinely cannot share, and batch everything else.

test_views_xform_prim.py is the one this run identified. Its
test_compare_get_world_poses_with_isaacsim reaches Isaac Sim's
SimulationManager, a process-global singleton that caches the PhysxScene
wrapping /physicsScene. In a shared process that prim belongs to a stage an
earlier file has already torn down, so the cached wrapper is dangling and the
test fails with "Accessed invalid expired 'PhysicsScene' prim". The other 62
tests in the file are fine; the marker is per file, so the file goes solo
until SimulationManager can be reset between files.

That leaves 26 of the directory's files sharing one app, up from 24, with two
marked kit_solo and one already in TESTS_TO_SKIP. The three kit_cameras files
are listed first in the batched job because a camera-enabled app can serve
tests that do not need cameras while the reverse makes launch_kit() raise.
Both jobs run the identical 26 so the durations stay comparable.
Both probe jobs carried a hand-written list of the files that can share a Kit
app, duplicated between them in two different formats. That has to be edited
by hand whenever a file is added, renamed, or reclassified, and the two
copies have to be kept identical or the timing comparison silently stops
comparing like with like. A stale list is wrong quietly rather than loudly.

The markers already record which files can share an app, so make them the
only source. tools/kit_test_files.py selects the files marked kit or
kit_cameras, drops those marked kit_solo and those in TESTS_TO_SKIP, and puts
the kit_cameras files first because a camera-enabled app can serve tests that
do not need cameras while the reverse makes launch_kit() raise. Each job
calls it in a step and passes the result through, so the two jobs cannot
disagree with each other or with the markers.

A marker expression still cannot replace this: pytest's -m deselects tests
but imports every collected module regardless, so it cannot stop a
kit_cameras module from calling launch_kit(cameras=True) in a run that booted
without cameras. The list has to be settled before pytest starts.

Markers are read from the source text rather than by importing the modules,
since importing a Kit-dependent test module boots Kit.

test_kit_marker_contract.py now also checks the two invariants a caller
depends on: the derived list matches the markers, and cameras sort first.
Verified the ordering check fails when the script's ordering is reversed.
kellyguo11 added a commit that referenced this pull request Aug 3, 2026
# Description

Fixes #5302.
Fixes #6853.

Manual direct workflows call the cloner after spawning their assets, but
most of them only authored per-environment collision groups on CPU—or
did not author them at all. The PhysX replication path currently uses
USD collision filtering instead of PhysX environment IDs, so CUDA
replicas could collide across environments and constrain articulated
joints despite valid effort targets.

This change:

- applies collision filtering on every PhysX simulation device for all
affected in-repo tasks and standalone scripts using the manual
direct-workflow cloning path;
- preserves each affected workflow's global ground or terrain collision
paths;
- guards the filtering by backend so Newton behavior is unchanged.

The deterministic 4,096-environment reproduction now reports identical
cart velocity in every environment after 20 steps at 100 N:

```text
min=6.7209768 m/s, max=6.7209768 m/s, mean=6.7209768 m/s
```

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

## Screenshots

Not applicable.

## Validation

- Audited standalone cloning scripts and updated the two affected
direct-workflow paths.
- Confirmed the original 4,096-environment constant-effort diagnostic
produces identical velocities across replicas.
- Confirmed the Newton MJWarp control remains consistent across four
replicas.
- `uv run isaaclab -f`
- `uv run --no-sync python tools/changelog/cli.py check issue-5302-base`
- Confirmed Cartpole rendering correctness passes for the PhysX/Isaac
Sim RTX and PhysX/Newton renderer combinations (2 passed).
- Confirmed the registered Cartpole and Shadow Hand camera tasks match
their updated collision-group stage goldens.

## Checklist

- [x] I have read and understood the contribution guidelines.
- [x] I have run the pre-commit checks.
- [x] Documentation changes are not required for this behavior-only fix.
- [x] My changes generate no new warnings.
- [x] Existing environment tests cover the affected behavior.
- [x] I have added a changelog fragment for the touched package.
- [x] My name already exists in `CONTRIBUTORS.md`.
The probe batched the kit_cameras files together with the plain kit ones, on
the assumption that a camera-enabled app is a superset: it can serve tests
that do not need cameras, so booting cameras first would satisfy everyone.

That is wrong, and CI showed exactly where. test_simulation_context.py's
test_headless_mode asserts

    not sim.has_gui and not sim.has_offscreen_render

so it fails in an app that was booted with cameras. The evidence is clean: in
the batch that contained no camera files that test passed 43/43, and in the
batch that booted cameras first it was the single failure out of 462 tests.

So the relationship is not superset but mutual exclusion. Cameras cannot be
enabled after startup, which rules out one order, and some tests require them
to be off, which rules out the other. Treat the two as separate batches.

launch_kit() now raises on any mismatch rather than only when cameras are
requested after a plain boot, so a file can never silently receive an app
configured differently from what its marker declares.

kit_test_files.py takes a --profile and returns one group, which also removes
the cameras-first ordering it previously had to arrange. Both probe jobs ask
for the kit group: it is much the larger, and a camera batch would mostly
measure the one-off ~600 s cold shader compile rather than Kit startup.

The contract test now checks each profile against the markers separately and
asserts the two groups do not overlap, replacing the ordering check that this
change makes meaningless.
matthewtrepte pushed a commit to matthewtrepte/IsaacLab that referenced this pull request Aug 4, 2026
# Description

Fixes isaac-sim#5302.
Fixes isaac-sim#6853.

Manual direct workflows call the cloner after spawning their assets, but
most of them only authored per-environment collision groups on CPU—or
did not author them at all. The PhysX replication path currently uses
USD collision filtering instead of PhysX environment IDs, so CUDA
replicas could collide across environments and constrain articulated
joints despite valid effort targets.

This change:

- applies collision filtering on every PhysX simulation device for all
affected in-repo tasks and standalone scripts using the manual
direct-workflow cloning path;
- preserves each affected workflow's global ground or terrain collision
paths;
- guards the filtering by backend so Newton behavior is unchanged.

The deterministic 4,096-environment reproduction now reports identical
cart velocity in every environment after 20 steps at 100 N:

```text
min=6.7209768 m/s, max=6.7209768 m/s, mean=6.7209768 m/s
```

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

## Screenshots

Not applicable.

## Validation

- Audited standalone cloning scripts and updated the two affected
direct-workflow paths.
- Confirmed the original 4,096-environment constant-effort diagnostic
produces identical velocities across replicas.
- Confirmed the Newton MJWarp control remains consistent across four
replicas.
- `uv run isaaclab -f`
- `uv run --no-sync python tools/changelog/cli.py check issue-5302-base`
- Confirmed Cartpole rendering correctness passes for the PhysX/Isaac
Sim RTX and PhysX/Newton renderer combinations (2 passed).
- Confirmed the registered Cartpole and Shadow Hand camera tasks match
their updated collision-group stage goldens.

## Checklist

- [x] I have read and understood the contribution guidelines.
- [x] I have run the pre-commit checks.
- [x] Documentation changes are not required for this behavior-only fix.
- [x] My changes generate no new warnings.
- [x] Existing environment tests cover the affected behavior.
- [x] I have added a changelog fragment for the touched package.
- [x] My name already exists in `CONTRIBUTORS.md`.
Files migrated to launch_kit() share the app when they land in the same
process, but the runner still gives every file its own subprocess, so the
sharing never happens and Kit startup is paid once per file. The temporary
probe measured what that costs: 23 files took 9m47s per-file against 5m29s
batched, a 44% reduction, with 17-18s of the per-file wall time being Kit
startup rather than tests.

Group files by launch profile and hand each group to pytest as one
invocation. tools/_kit_batching.py decides the grouping and takes the
resulting JUnit report back apart per file, so the summary table, the
failed-file list, and the uploaded artifact stay keyed by file exactly as
before. Both are pure functions over paths and strings, which is why they can
be tested on any platform while the process machinery around them cannot.

Off unless ISAACLAB_TEST_BATCH_KIT is set. The per-file path is untouched and
remains the default.

Kept out of batches: unmarked files, kit_solo, device_split files (already
invoked once per device with different -k), files with node-ID selection,
the visualizer files that are retried in a fresh process, and anything whose
own timeout reaches 2000s. A batch's timeout is the sum of its members', so
one hang would consume the whole budget -- and those long files are exactly
where Kit startup is a rounding error, so excluding them drops most of the
risk and almost none of the gain. Batching is also disabled under the work
queue, which hands out files one at a time and cannot offer coherent groups.

When a batch dies early the files it never reached are re-run individually,
so batching degrades to the behaviour it replaces rather than losing results.

Batches carry an index because the label becomes a JUnit report filename:
without it, two same-profile batches of equal size collided on one path and
the second silently overwrote the first. There is a regression test for that.

The probe jobs are removed; they were scaffolding for the measurement above
and were re-running the same files a second and third time on every PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant